Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix reporting IsFromComputationExpression for inappropriate symbols #17375

Merged
merged 9 commits into from
Jul 24, 2024
Merged

Fix reporting IsFromComputationExpression for inappropriate symbols #17375

merged 9 commits into from
Jul 24, 2024

Conversation

DedSec256
Copy link
Contributor

@DedSec256 DedSec256 commented Jul 2, 2024 •
edited
Loading

Description

Upon investigating the syntax highlighting issue for CE code in Rider, I found that FCS reports a separate SymbolUse for expressions that are not a constructor invocation of the CE builder type or an associated with it let-binding:

image

This behavior seems incorrect because the Object.Prop expression does not appear to require highlighting as a CE keyword. Unexpected case, and therefore it may not be properly handled, which can lead to incorrect overlapped code highlighting, for example, like this

image

or even worse

image

and can also lead to navigation errors in the 'go to definition' due to overlapping SymbolUse ranges.

The same issue is reproducible in VS/VS Code.

fsharp/src/Compiler/Service/FSharpCheckerResults.fs

Lines 236 to 242 in 5c6d8e7

member _.IsFromComputationExpression =
match symbol.Item, itemOcc with
// 'seq' in 'seq { ... }' gets colored as keywords
| Item.Value vref, ItemOccurence.Use when valRefEq denv.g denv.g.seq_vref vref -> true
// custom builders, custom operations get colored as keywords
| (Item.CustomBuilder _ | Item.CustomOperation _), ItemOccurence.Use -> true
| _ -> false

Also, I did not find any tests confirming that IsFromComputationExpression should be true for such expressions.

Fix

This PR suggests reporting IsFromComputationExpression only for CE Builder type constructors and let-bindings.

Checklist

  • Test cases added
  • Release notes entry updated

@DedSec256 DedSec256 requested a review from a team as a code owner July 2, 2024 00:18
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 2, 2024 •
edited
Loading

❗ Release notes required

✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/9.0.100.md

@DedSec256 DedSec256 force-pushed the ber.a/fixCeBuilderReporting branch from 5e7c8ac to c14a435 Compare July 2, 2024 00:32
@brianrourkeboll
Copy link
Contributor

Hmm, VS does not colorize things like A.Builder { return 3 } or System.Object.Builder { return 3 } as keywords. Does Rider currently do so? Or are you just saying that the extra SymbolUse shouldn't be there?

image

@DedSec256
Copy link
Contributor Author

DedSec256 commented Jul 3, 2024 •
edited
Loading

@brianrourkeboll,

You can notice incorrect coloring of parentheses as CE keyword color there.

image

It also seems incorrect for a more complex case (tested on VS v17.10.3):

image

There is also the issue in VS Code:

image

I did not research the logic of how Visual Studio/VS Code maps symbols to highlightings; however, the extra SymbolUse with IsFromComputationExpression indeed shouldn't be listed for cases from this PR.

As a result, it will also fix highlightings in Rider (and, hopefully, in VS/VS Code)

PS. Thanks, I updated the PR description a little bit

@DedSec256
Copy link
Contributor Author

DedSec256 commented Jul 3, 2024 •
edited
Loading

Perhaps this case is OK 🤔

image

@brianrourkeboll
Copy link
Contributor

Perhaps this case is OK 🤔

image

Hmm, yeah, I mean how far do we want to go? :)

image

@baronfel
Copy link
Member

baronfel commented Jul 3, 2024

For context, here's how FSAC does semantic tokenization.

Generally we take the classifications directly from FCS via the SemanticClassificationType that is determined for a range, but we have to map those SemanticClassificationTypes to a set of SemanticTokenType and SemanticTokenModifier that are defined partially in the LSP spec and partially in FSAC's LSP bindings. This combination of token type + token modifier is mapped to predefined and user-defined editor styling.

So in general we assume FSC is true/correct, and try to do the most minimal mapping/handling possible for this feature area.

@T-Gro
Copy link
Member

T-Gro commented Jul 8, 2024

@brianrourkeboll,

You can notice incorrect coloring of parentheses as CE keyword color there.

image

It also seems incorrect for a more complex case (tested on VS v17.10.3):

image

There is also the issue in VS Code:

image

I did not research the logic of how Visual Studio/VS Code maps symbols to highlightings; however, the extra SymbolUse with IsFromComputationExpression indeed shouldn't be listed for cases from this PR.

As a result, it will also fix highlightings in Rider (and, hopefully, in VS/VS Code)

PS. Thanks, I updated the PR description a little bit

I like this change @DedSec256 .
Since you already created samples+screenshots showing the issue, could you do the exact same snippets with the included fix?

At least the VS once, by starting it in VisualFsharp.sln

@DedSec256
Copy link
Contributor Author

DedSec256 commented Jul 8, 2024 •
edited
Loading

@T-Gro, have checked before and after the fix. The issue is resolved.

Before
image

After
image

As you can see, the unit parameter of the constructor is still being highlighted in blue, but I think this is a separate issue; I don't observe the same problem in Rider/VS Code.

psfinaki
psfinaki approved these changes Jul 8, 2024
View reviewed changes
Copy link
Member

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job! Thanks for adding a test there as well.

@psfinaki psfinaki enabled auto-merge (squash) July 24, 2024 10:44
@psfinaki
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@psfinaki
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@psfinaki
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@psfinaki psfinaki merged commit db4ad94 into dotnet:main Jul 24, 2024
30 checks passed
This was referenced Jul 31, 2024
Closed
Closed
@dotnet-bot dotnet-bot mentioned this pull request Aug 28, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@T-Gro T-Gro T-Gro approved these changes

@edgarfgp edgarfgp edgarfgp approved these changes

@psfinaki psfinaki psfinaki approved these changes

@auduchinok auduchinok auduchinok approved these changes

Assignees
No one assigned
Labels
None yet
Projects
F# Compiler and Tooling
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@DedSec256 @brianrourkeboll @baronfel @T-Gro @psfinaki @auduchinok @edgarfgp